Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase default project quota in the sandbox and demo #3061

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Nov 8, 2022

Signed-off-by: Kevin Su [email protected]

Tracking issue

https://flyte-org.slack.com/archives/C049Q7GDWN9/p1667885776403149

Describe your changes

People often fail to run the workflow in the sandbox or demo cluster because resource exceeds the task default resources or the project quota. Therefore, we increase the quota and default resource to address this issue.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw requested a review from eapolinario November 9, 2022 20:47
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you test this, Kevin?

@@ -102,3 +117,64 @@ redis:
# --- enable or disable Redis Statefulset installation
enabled: false

# -- Configuration for the Cluster resource manager component. This is an optional component, that enables automatic
# cluster configuration. This is useful to set default quotas, manage namespaces etc that map to a project/domain
cluster_resource_manager:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, If we don't add it, the sandbox will use the default config in value.yaml

How did you test this, Kevin?

I rebuilt the sandbox image and ran the command flytectl sandbox start --image pingsutw/sandbox-test to create cluster. Then check if the resource is updated in the configmap.

@hamersaw
Copy link
Contributor

I'm happy about this - I've run into OOM issue during local development as well. Only concern is maybe we're bumping these too high? Ex. we probably don't need a quota for 40 CPU and 30G memory for local development. Maybe something like:

  • task resource defaults 500m CPU requests and 2 CPU limits / 1G memory requests and 4G memory limits
  • quotas likes 8CPU and 16GB memory

Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw requested a review from eapolinario November 16, 2022 22:56
limits:
cpu: 2
memory: 4Gi
storage: 20Mi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this so low? Do we have to set it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the default value in value.yaml. Yup, it's optional. I just removed it to make the config simple.

@pingsutw pingsutw merged commit fa4ca80 into master Nov 17, 2022
@pingsutw pingsutw deleted the update-quota branch November 17, 2022 22:18
@gpgn
Copy link

gpgn commented Jul 7, 2023

Hi, I had some issues with the default demo environment you get when running flytectl demo start, see discussion here: https://flyte-org.slack.com/archives/CP2HDHKE1/p1688567606055059

I was able to prevent things from getting OOMKilled by updating the sandbox configmap according to above values. Would it make sense for you to use these as defaults for the demo environment as well? Let me know if I should open a separate issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants